Skip to content

Installer - Use Riverlea's Minetta by default - #31985

Merged
ufundo merged 1 commit into
civicrm:6.0from
totten:6.0-theme-install
Feb 11, 2025
Merged

Installer - Use Riverlea's Minetta by default#31985
ufundo merged 1 commit into
civicrm:6.0from
totten:6.0-theme-install

Conversation

@totten

@totten totten commented Feb 7, 2025

Copy link
Copy Markdown
Member

Overview

In keeping with the 6.0 recommendation for Riverlea... use it on new sites (by default).

This is an alternative implementation of #31979 (which should be less likely to anger the testgods).

Before

New installations use Greenwich

After

New installations use Minetta.

@civibot

civibot Bot commented Feb 7, 2025

Copy link
Copy Markdown

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@ufundo

ufundo commented Feb 7, 2025

Copy link
Copy Markdown
Contributor

I can see the expediency of this.

But it feels like its "set Minetta as the theme" rather than "make Minetta the default". And it will force everyone to an explicit setting, and I have a sense retaining some distinction between folks on the implicit default for the setting vs people who have explicit chosen a value.

Possible third way: #31986

@totten

totten commented Feb 8, 2025

Copy link
Copy Markdown
Member Author

@ufundo Step back and consider the general architecture of Civi and Riverlea. Civi is deployed in many environments (CMS's, standalone, frontend/backend, etc). Riverlea is a base-theme with a library of sub-themes, and it's bundled-in. What's the point of bundling-in an entire library of themes? (Clearly, 3 themes is more work than 1 theme!)

Well, different themes work better in different cases. Prime example: the typical UIs in WordPress, Drupal, and Joomla look different. (The ideal default for WP-admin differs from the ideal default for Joomla-admin.) In the end-game, we're juggling multiple recommendations.

The ultimate default requires logic, which could be in a few different places:

  • That logic could be runtime logic. (The current theme_xxx=default accommodates that -- you can reinterpret default in subjective ways.)
    • Of course, as you astutely pointed out, we've never had the gumption to actually change the interpretation of theme_xxx=default.
    • The lesson I see is: the theme is a big deal. Changing theme_xxx is a much more visible/complicated/subjective affair than changing (say) max_attachments. If we exercise the prerogative to change default, then the blast-radius will be too big.
  • That logic could be in the installer / upgrader / status-checks. (These tools manage an explicit value -- which we can compute/override/preserve in various ways.)
    • This model is more work (for us) than a declarative default (like Civi\Core\Themes::DEFAULT_THEME = greenwich or like Core.setting.php with default=>minetta in Riverlea rollout - set Minetta as default, freeze previous default of Greenwich, sidestep DEFAULT_THEME #31986).
    • It's also more work (for us) than runtime-computation (like theme_xxx=default).
    • But it is nevertheless a predictable/familiar game.
    • This model gives more precise control over the blast-radius. Transitions can be staged.
    • It's more proportionate to the expectations/requirements for themes.

(^^ Edited a little for clarity.)

I can see the expediency of this... But... force everyone to an explicit setting

  • Yeah, it is expedient.
  • It's also future-aligned. The value should be explicit (augmented by installer/upgrader/status-checks) -- because the theme-setting is a big deal, and we should be able to control the blast-radius anytime we revise it.
  • I suspect you feel a bit underwhelmed looking at this patch -- because it's not comprehensive.
    • This doesn't clear-out the tight coupling to Greenwich.
    • I completely agree with that goal.
    • We should do that.
    • It doesn't need to be at the time of 6.0 freeze. It's more complicated. (I'd rather push that to 6.1 or 6.2 than ship a rushed/partial/buggy variant in 6.0.)

@ufundo

ufundo commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

The value should be explicit (augmented by installer/upgrader/status-checks)

Interesting. This is the crux for me.

I am not underwhelmed by this patch, I'm worried it overreaches... the "making minetta the explicit setting" is a one-line patch, but is something we cannot step back from.

I'm fairly convinced by what you've said above...

@ufundo

ufundo commented Feb 10, 2025

Copy link
Copy Markdown
Contributor

we should be able to control the blast-radius anytime we revise it

So was thinking about this some more. I think making it explicit is in practice always going to restrict the blast radius of future changes. That may be appropriate - but it's not "this change allows us to choose whatever blast radius we want going forward".

One group we are foregoing the ability to blast is "people who only have Minetta because the 6.0 upgrader gave it to them".

@ufundo

ufundo commented Feb 11, 2025

Copy link
Copy Markdown
Contributor

Ok, I'm a still a little nervous, but (as Mike Skinner once said) let's push things forward d

@ufundo
ufundo merged commit ca70dc2 into civicrm:6.0 Feb 11, 2025
@totten
totten deleted the 6.0-theme-install branch February 12, 2025 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants